-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow ?
and *
parameters to be omitted in tail position
#815
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗺️ PR Tour 🧭
pub fn match_empty_arg_group(self) -> IonMatchResult<'top> { | ||
recognize(pair(tag("(:"), whitespace_and_then(tag(")"))))(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗺️ The original impl was just returning the matched slice. Now there's a concrete type that represents an argument group, so we instantiate one of those and return it.
($parser:ident $macro_src:literal $($expect:ident: [$($input:literal),+$(,)?]),+$(,)?) => { | ||
mod $parser { | ||
($mod_name:ident $parser:ident $macro_src:literal $($expect:ident: [$($input:literal),+$(,)?]),+$(,)?) => { | ||
mod $mod_name { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗺️ This macro creates a module with a bunch of nested unit tests. However, the module name was also the name of the parser. This meant you couldn't write two sets of unit tests that used the same parser entry point. I added another parameter ($mod_name
) to this macro to allow the module name to be specified separately.
@@ -2967,6 +2975,7 @@ mod tests { | |||
} | |||
|
|||
matcher_tests_with_macro! { | |||
parsing_sexps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗺️ In the next few tests, I added a distinct module name.
allow_omitting_trailing_optionals | ||
match_e_expression | ||
"(macro foo (a b+ c? d*) null)" | ||
expect_match: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding an expectation that the explicit empty argument group (:)
also works?
expect_match: [ | ||
"(:foo 1 2)", | ||
"(:foo 1 2 3)", | ||
"(:foo 1 2 3 4)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"(:foo 1 2 3 4)", | |
"(:foo 1 2 3 4)", | |
"(:foo 1 2 (: 3) 4)", | |
"(:foo 1 2 3 (: 4))", |
...unless these are already covered elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text reader doesn't have non-empty arg group support yet.
"(:foo 1 2)", | ||
"(:foo 1 2 3)", | ||
"(:foo 1 2 3 4)", | ||
"(:foo 1 2 3 4 5 6)", // implicit rest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"(:foo 1 2 3 4 5 6)", // implicit rest | |
"(:foo 1 2 3 4 5 6)", // implicit rest | |
"(:foo 1 2 (: 3) 4 5 6)", // implicit rest | |
"(:foo 1 2 3 (: 4 5 6))", |
...unless these are already covered elsewhere.
], | ||
expect_mismatch: [ | ||
"(:foo 1)", | ||
"(:foo)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"(:foo)", | |
"(:foo)", | |
"(:foo 1 2 3 4 (: 5) 6)", // final argument must be rest or expression group, but not a mix of both |
...unless this is already covered elsewhere.
Previously the text reader only allowed arguments that could use rest syntax to also be omitted altogether.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.